Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix lint issues #4545

Merged
merged 1 commit into from
Jul 22, 2024
Merged

Fix lint issues #4545

merged 1 commit into from
Jul 22, 2024

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Jul 22, 2024

Explanation

For some reason, two lint violations show up on my computer when running yarn lint:eslint. I have attempted to remove node_modules, remove all dist directories, remove .eslintcache, run yarn cache clear, etc. to no avail. I think an ultimate fix here may be to upgrade ESLint packages but that would be a larger change, so as egregious as it is, this commit skirts around the issue by adding some ignore lines.

References

Intermittent lint violations have been a problem for a long time. Also see #4540.

Changelog

(N/A)

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate

For some reason, two lint violations show up on my computer when running
`yarn lint:eslint`. I have attempted to remove `node_modules`, remove
all `dist` directories, remove `.eslintcache`, run `yarn cache clear`,
etc. to no avail. I think an ultimate fix here may be to upgrade ESLint
packages but that would be a larger change, so as egregious as it is,
this commit skirts around the issue by adding some ignore lines.
@Mrtenz
Copy link
Member

Mrtenz commented Jul 22, 2024

Did you try restarting your local TypeScript language server?

@mcmire
Copy link
Contributor Author

mcmire commented Jul 22, 2024

@Mrtenz Yeah, I just tried closing my editor, running killall node, explicitly looking for tsserver and killing that, and when I run yarn lint:eslint I'm still getting the two violations.

@mcmire mcmire merged commit 0f80f16 into main Jul 22, 2024
116 checks passed
@mcmire mcmire deleted the fix-lint-issues branch July 22, 2024 17:26
@Gudahtt
Copy link
Member

Gudahtt commented Jul 22, 2024

Not a huge fan of this idea. This type of rule exclusion prevents us from using the reportUnusedDIsableDirectives rule, which would let us treat ESLint ignore comments more like ts-expect-error, in that it would flag cases where they're obsolete.

@mcmire
Copy link
Contributor Author

mcmire commented Jul 22, 2024

@Gudahtt Sure. I wanted to quickly unblock other people if they were encountering this, too, but I admit that this is probably not the best way to deal with this. Is there another way that you would suggest? Are you concerned that we don't currently have a good way to remove these later?

@Gudahtt
Copy link
Member

Gudahtt commented Jul 22, 2024

I'd suggest that we investigate this further and find the root cause.

I am not sure what you meant by your last question.

@mcmire
Copy link
Contributor Author

mcmire commented Jul 22, 2024

@Gudahtt You mentioned the use of reportUnusedDisableDirectives. This rule (as I understand it) looks for eslint-disable directives that are unnecessary (because the line that they are referring to no longer has a lint violation). If we were to enable this option and we found that the lint violations I found mysteriously went away, then reportUnusedDisableDirectives should alert us to that so that we can remove those disable directives. Is that true? Or was there some other reason that you mentioned this option?

@Gudahtt
Copy link
Member

Gudahtt commented Jul 22, 2024

I mentioned that rule because enabling it would require undoing this change, leaving contributors in the same state of having local lint errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants